-
Notifications
You must be signed in to change notification settings - Fork 117
feat: add fmt_engineering()
#786
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #786 +/- ##
==========================================
+ Coverage 91.61% 91.77% +0.15%
==========================================
Files 47 47
Lines 5773 5821 +48
==========================================
+ Hits 5289 5342 +53
+ Misses 484 479 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Code review (updated)MICHAEL NOTES:
Found 7 issues, ordered by confidence: High confidence (80+):
great-tables/great_tables/_formats.py Lines 1111 to 1115 in cec2fb0
great-tables/great_tables/_formats_vals.py Lines 20 to 22 in cec2fb0
Medium confidence (50-79):
great-tables/great_tables/_formats.py Line 1077 in cec2fb0
great-tables/great_tables/_formats.py Lines 1095 to 1100 in cec2fb0
Lower confidence (< 50):
great-tables/great_tables/vals.py Lines 5 to 40 in cec2fb0
great-tables/tests/test_formats.py Lines 1165 to 1196 in cec2fb0
great-tables/tests/test_formats.py Lines 1250 to 1286 in cec2fb0
Generated with Claude Code If this review was useful, please react with 👍. Otherwise, react with 👎. |
Removed the unused
The import style changes in
Done. Simplified test cases significantly. Reduced from ~70 test values down to ~30 while maintaining coverage of key behaviors (positive/negative values, extreme magnitudes, zero handling, all exp_styles, force_sign options, etc.).
Fixed. I completely removed the For engineering notation, the mantissa ranges from Note that
Done. Branch has been rebased onto main and
Fixed. Removed the local
The existing
Fixed. Reverted to the grouped import style matching main branch.
Addressed. Reduced test inputs to minimum needed to demonstrate each feature while still covering key edge cases (boundary values, positive/negative, zero, extreme magnitudes).
Addressed. Reduced |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks this looks great! The one final thing I might suggest from reading the docstring
I wonder if it'd be helpful to frontload an example, like...
With numeric values in a table, we can perform formatting so that the targeted values are rendered in engineering notation. For example, the number
0.0000345in engineering notation can be34.50 x 10^-6. Engineering notation represents numbers as a mantissa (m) and an exponent (n), in the formm x 10^normEn. ...
Essentialy, moving the example from the end to the front, so it becomes a worked example
This PR adds the
.fmt_engineering()formatting method. Engineering notation expresses values so that they align to certain SI prefixes. Here is a table that compares select SI prefixes and their symbols to decimal and engineering-notation representations of key numbers.Fixes: #785